fix(server): use vim.system instead of jobstart to bypass shell#560
Conversation
|
Hi @JonLD, thanks for the contribution! Have you considered replacing |
|
Hi @jakubbortlik, thanks for the suggestion! I've gone ahead and pushed a change to use |
|
Hi @JonLD, can you please rebase and update the PR description. |
Pass the server binary and JSON settings as a list to vim.fn.jobstart instead of a single formatted string. List form launches the process directly via OS calls, bypassing the user's $SHELL and its parsing rules. Removes the need for the win32-specific quote escaping, since no shell sees the JSON argument anymore. Fixes failures on Windows with non-POSIX shells (e.g. nushell), which reject the backslash-escaped quotes the previous code emitted.
43aec6c to
d9e326a
Compare
|
Hi @jakubbortlik, rebased and updated PR description. |
| stderr = function(_, data) | ||
| if data == nil or data == "" then | ||
| return | ||
| end | ||
| vim.schedule(function() | ||
| u.notify(data, vim.log.levels.ERROR) | ||
| end) | ||
| end, |
There was a problem hiding this comment.
Hi @JonLD, I'va had Claude review this and it says:
Previously stderr was accumulated with List.reduce and shown as one message. Now each
chunk fires a separate u.notify.
If the Go server writes a multi-line error across two chunks, the user sees two separate
notifications. For a long stacktrace this can be noisy. Simple fix: concatenate into a
module-scoped buffer and flush on nil:
local stderr_buf = ""
stderr = function(_, data)
if data == nil then
if stderr_buf ~= "" then
local msg = stderr_buf
stderr_buf = ""
vim.schedule(function() u.notify(msg, vim.log.levels.ERROR) end)
end
return
end
stderr_buf = stderr_buf .. data
end,
Could you please verify that your modification is not a regression from the previous behaviour? I can confirm that now when I for example mess up the server settings, I'm getting two messages:
gitlab.nvim: Failure parsing plugin settings: invalid character 'B' after top-level value
gitlab.nvim: Golang gitlab server exited: code: 1, signal: 0
With the following patch, I get only one message (gitlab.nvim: Golang gitlab server exited: code: 1, signal: 0, msg: Failure parsing plugin settings: invalid character 'B' after top-level value) which I would prefer to see, if it's just one problem:
diff --git a/lua/gitlab/server.lua b/lua/gitlab/server.lua
index b06c5060..9d2e6a4e 100644
--- a/lua/gitlab/server.lua
+++ b/lua/gitlab/server.lua
@@ -62,6 +62,7 @@ M.start = function(callback)
local settings = vim.json.encode(go_server_settings)
+ local stderr_buf, msg = "", ""
local ok, err = pcall(vim.system, { state.settings.server.binary, settings }, {
stdout = function(_, data)
if data == nil or parsed_port ~= nil then
@@ -84,18 +85,20 @@ M.start = function(callback)
end
end,
stderr = function(_, data)
- if data == nil or data == "" then
+ if data == nil then
+ if stderr_buf ~= "" then
+ msg = stderr_buf
+ stderr_buf = ""
+ end
return
end
- vim.schedule(function()
- u.notify(data, vim.log.levels.ERROR)
- end)
+ stderr_buf = stderr_buf .. data
end,
}, function(out)
if out.code ~= 0 then
vim.schedule(function()
u.notify(
- "Golang gitlab server exited: code: " .. out.code .. ", signal: " .. (out.signal or 0),
+ "Golang gitlab server exited: code: " .. out.code .. ", signal: " .. (out.signal or 0) .. ", msg: " .. msg,
vim.log.levels.ERROR
)
end)There was a problem hiding this comment.
Hi @jakubbortlik, nice spot. I don't think this is a regression with my changes as on the head of develop you still get separate notifications for "Golang gitlab server exited" and the failure message. That said it does make sense to just combine them into one notification so I've gone ahead and made the change. It's slightly different to the diff you suggested but achieves the same.
There was a problem hiding this comment.
I think the point of the "regression" was that your original stderr implementation would have notified for each "chunk of data":
stderr = function(_, data)
if data == nil or data == "" then
return
end
vim.schedule(function()
u.notify(data, vim.log.levels.ERROR)
end)
end,The fact that both stderr and on_exit notified separately was a separate (and indeed pre-existing) issue.
Now your solutio looks much cleaner than my suggestion! The only thing I've noticed is that now the message can contain a trailing new line character that e.g., in Snacks notification history show as blank lines:

Could you please trim the final new lines?
There was a problem hiding this comment.
Ahh I see your point with the regression, didn't notice that in any testing.
Trailing newline trimmed now.
|
Thanks for the PR! |
Summary
Switch the server launch from
vim.fn.jobstarttovim.system, passing the binary and JSON settings as a list. Neovim spawns the process directly instead of going through the user's shell, removing the Windows-specific"-escaping hack that only worked for POSIX-style shells.Why
On Windows with a non-POSIX shell (e.g. Nushell) the previous string-form
jobstartcall was passed through&shell &shellcmdflag, and the quoting/escaping assumptions broke, preventing the Go server from starting.vim.systemskips the shell entirely, so argument boundaries and JSON quoting are preserved verbatim regardless of shell.Test plan
shell=nuand confirm the server launches